Skip to content

Add vec::Drain{,Filter}::keep_rest #95376

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Aug 30, 2022
Merged

Add vec::Drain{,Filter}::keep_rest #95376

merged 3 commits into from
Aug 30, 2022

Conversation

@WaffleLapkin
Copy link
Member

@WaffleLapkin WaffleLapkin commented Mar 27, 2022 •
edited
Loading

This PR adds keep_rest methods to vec::Drain and vec::DrainFilter under drain_keep_rest feature gate:

// mod alloc::vec

impl<T, A: Allocator> Drain<'_, T, A> {
    pub fn keep_rest(self);
}

impl<T, F, A: Allocator> DrainFilter<'_, T, F, A>
where
    F: FnMut(&mut T) -> bool,
{
    pub fn keep_rest(self);
}

Both these methods cancel draining of elements that were not yet yielded from the iterators. While this needs more testing & documentation, I want at least start the discussion. This may be a potential way out of the "should DrainFilter exhaust itself on drop?" argument.

@rust-highfive
Copy link
Contributor

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 27, 2022
@Gankra
Copy link
Contributor

Gankra commented Mar 27, 2022

I haven't looked at the code, but I think this is a better answer to 90% of what drain_filter is trying to do, and the appropriate release valve for people who don't like drain's default semantic of "you have already drained all of the elements, now we shall yield them one at a time". It is a very "Entry" solution to the problem in that it provides an external interface to drain_filter's messy internal one... oh god is that terminology anyone uses anymore. crumbles to dust

@WaffleLapkin
Copy link
Member Author

Truth be told, when I struggled with the default behavior of drain_filter I've implemented a crappy entry/cursor API for Vec. I guess Entry-like APIs are just superior answer to all problems.

@bors
Copy link
Collaborator

bors commented Apr 8, 2022

☔ The latest upstream changes (presumably #95798) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon
Copy link
Member

Ping from triage:
@WaffleLapkin can you please resolve the merge conflicts?

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 8, 2022
@the8472
Copy link
Member

the8472 commented May 21, 2022

I think this should be the default for DrainFilter rather than an additional method.

Drain on the other hand already is stable so we can't change the default behavior and adding a method makes more sense there. But it's awkward ergonomically because those are iterators and you're presumably using them rather than retain() because you actually need the iterator part so you'd have to take it by_ref().

Perhaps adding a defaulted const generic Drain<T, const KEEP: bool = false> and keep_rest(self) -> Drain<T, true> would be more ergonomic.

@the8472 the8472 added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 21, 2022
@the8472
Copy link
Member

the8472 commented May 21, 2022

r? rust-lang/libs-api

@rust-highfive rust-highfive assigned dtolnay and unassigned kennytm May 21, 2022
@bors
Copy link
Collaborator

bors commented Jun 4, 2022

☔ The latest upstream changes (presumably #97729) made this pull request unmergeable. Please resolve the merge conflicts.

@WaffleLapkin
Add vec::Drain{,Filter}::keep_rest
50c98a8 
These methods allow to cancel draining of unyielded elements.
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 3, 2022
dtolnay
dtolnay reviewed Aug 27, 2022
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am on board with this API. Next steps are:

  • I think this is the kind of method that would benefit from a usage example in the rustdoc.
  • Needs a tracking issue.

Some considerations to note in the tracking issue for discussion before we could stabilize:

  • Just change the not-yet-stable Drop for DrainFilter to keep rest?
    • Advantage: usually what you want (??)
      • I don't have a good picture what the use cases involve drain_filter and not exhausting the iterator obtained from it.
    • Disadvantage: inconsistent with stable Drop for Drain
    • If you want to remove rest (matching the current unstable behavior of drain_filter) then you'd need to write .foreach(drop) to explicitly drop all the rest of the range that matches the filter.
  • &mut self instead of self?
    • If you're holding a Drain inside a struct and are operating on it from a &mut self method of the struct, keep_rest(self) is impossible to use. :(
      • You'd want something like mem::replace(&mut self.drain_filter, Vec::new().drain(..)).keep_rest() but the borrow checker won't like that.
      • Failing that, you'd need to change your Drain field to Option<Drain> and use self.drain_filter.take().unwrap().keep_rest() along with unwrap() everywhere else that the drain is used. Not good.
    • We'd need to define behavior of calling .next() after .keep_rest(). Presumably one of:
      • .next() returns None
      • this is considered incorrect usage and so .next() panicks
      • .keep_rest() sets a flag inside the Drain which Drop will use to decide its behavior, and you're free to continue accessing iterator elements in between.

@WaffleLapkin WaffleLapkin mentioned this pull request Aug 28, 2022
Open
3 tasks
@WaffleLapkin
Copy link
Member Author

I've updated docs and the tracking issue ✅

dtolnay
dtolnay approved these changes Aug 29, 2022
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@dtolnay
Copy link
Member

dtolnay commented Aug 29, 2022

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 29, 2022

📌 Commit 7a433e4 has been approved by dtolnay

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 29, 2022
@Dylan-DPC Dylan-DPC mentioned this pull request Aug 30, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 30, 2022
@bors
Auto merge of rust-lang#101183 - Dylan-DPC:rollup-6kewixv, r=Dylan-DPC
0631ea5 
Rollup of 9 pull requests

Successful merges:

 - rust-lang#95376 (Add `vec::Drain{,Filter}::keep_rest`)
 - rust-lang#100092 (Fall back when relating two opaques by substs in MIR typeck)
 - rust-lang#101019 (Suggest returning closure as `impl Fn`)
 - rust-lang#101022 (Erase late bound regions before comparing types in `suggest_dereferences`)
 - rust-lang#101101 (interpret: make read-pointer-as-bytes a CTFE-only error with extra information)
 - rust-lang#101123 (Remove `register_attr` feature)
 - rust-lang#101175 (Don't --bless in pre-push hook)
 - rust-lang#101176 (rustdoc: remove unused CSS selectors for `.table-display`)
 - rust-lang#101180 (Add another MaybeUninit array test with const)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c731157 into rust-lang:master Aug 30, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 30, 2022
@WaffleLapkin WaffleLapkin deleted the drain_keep_rest branch February 6, 2025 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dtolnay dtolnay dtolnay approved these changes

Assignees

@dtolnay dtolnay

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.65.0

Development

Successfully merging this pull request may close these issues.

9 participants

@WaffleLapkin @rust-highfive @Gankra @bors @JohnCSimon @the8472 @dtolnay @kennytm @rustbot